feat: add Federation allow list #37010
Conversation
|
Looks like this PR is not ready to merge, because of the following issues:
Please fix the issues and try again If you have any trouble, please check the PR guidelines |
|
WalkthroughAdds a new non-public Federation_Service_Allow_List setting, introduces a middleware to enforce an allow-listed set of federation domains, wires that middleware into the FederationMatrix route chain, and adds corresponding English i18n strings. No public API signatures changed. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant C as Federation Peer
participant FM as FederationMatrix Router
participant L as isLicenseEnabledMiddleware
participant A as isFederationDomainAllowedMiddleware
participant E as Matrix Endpoint (invite/profile/rooms)
C->>FM: HTTP request + Authorization header
FM->>L: Check license
alt License disabled
L-->>C: 403/disabled
else License enabled
L->>A: Next
alt Allow-list empty
A->>E: Next
E-->>C: Endpoint response
else Allow-list present
alt Missing/invalid Authorization or origin
A-->>C: 401 (M_UNAUTHORIZED / M_MISSING_ORIGIN)
else Origin allowed
A->>E: Next
E-->>C: Endpoint response
else Origin not allowed
A-->>C: 403 (M_FORBIDDEN)
end
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
ee/packages/federation-matrix/src/FederationMatrix.ts (1)
165-176: Expose public federation endpoints before the allow-list middlewareMatrix federation key and version endpoints must be reachable without auth/allow-list checks — move getKeyServerRoutes and getFederationVersionsRoutes ahead of isFederationDomainAllowedMiddleware to avoid breaking federation handshakes.
File: ee/packages/federation-matrix/src/FederationMatrix.ts
Lines: 165-176matrix .use(isFederationEnabledMiddleware) .use(isLicenseEnabledMiddleware) - .use(isFederationDomainAllowedMiddleware) + // Public endpoints must remain accessible without auth/allow-list checks + .use(getKeyServerRoutes(this.homeserverServices)) + .use(getFederationVersionsRoutes(this.homeserverServices)) + // Enforce allow-list for S2S endpoints that require auth + .use(isFederationDomainAllowedMiddleware) .use(getMatrixInviteRoutes(this.homeserverServices)) .use(getMatrixProfilesRoutes(this.homeserverServices)) .use(getMatrixRoomsRoutes(this.homeserverServices)) .use(getMatrixSendJoinRoutes(this.homeserverServices)) .use(getMatrixTransactionsRoutes(this.homeserverServices)) - .use(getKeyServerRoutes(this.homeserverServices)) - .use(getFederationVersionsRoutes(this.homeserverServices)); + ;
🧹 Nitpick comments (4)
packages/i18n/src/locales/en.i18n.json (1)
2160-2161: Align label casing and clarify input format for consistency
- Current label casing is inconsistent with nearby Federation labels.
- Description should state format/behavior (comma-separated; blank behavior).
Suggested tweak:
- "Federation_Service_Allow_List": "Domain Allow List", - "Federation_Service_Allow_List_Description": "Restrict federation to the given allow list of domains.", + "Federation_Service_Allow_List": "Federation domain allow list", + "Federation_Service_Allow_List_Description": "Comma-separated list of domains allowed for federation. Leave blank to allow all."Please confirm the actual parsing/behavior (whitespace trimming, case sensitivity, wildcard support, and what an empty value means) and we can adjust wording accordingly.
apps/meteor/server/settings/federation-service.ts (1)
28-33: Clarify expected format and scope of the allow list.Please clarify in the i18n description that this setting expects a comma-separated list of domains (no protocol, no ports), and whether entries may begin with a leading dot to cover subdomains. This reduces misconfiguration risk.
Would you like me to open a follow-up PR updating
packages/i18n/src/locales/en.i18n.jsonto reflect this?ee/packages/federation-matrix/src/api/middlewares/isFederationDomainAllowed.ts (2)
43-47: Skip allow-list checks for public endpoints (defense in depth).Even with route reordering, add a path guard so
/_matrix/key/*and/_matrix/versionsremain accessible if this middleware is ever applied earlier.Apply this diff:
export const isFederationDomainAllowedMiddleware = createMiddleware(async (c, next) => { const allowList = await getAllowList(); if (!allowList || allowList.length === 0) { // No restriction, allow all return next(); } + // Public endpoints must bypass allow-list checks + const p = c.req.path; + if (p.startsWith('/_matrix/key/') || p === '/_matrix/versions') { + return next(); + } + // Extract all key-value pairs from Matrix authorization header const authHeader = c.req.header('authorization'); if (!authHeader) { return c.json({ errcode: 'M_UNAUTHORIZED', error: 'Missing Authorization headers.' }, 401); }
6-17: Harden allow-list entries for consistent comparison.Strip leading/trailing dots from entries so
.example.comandexample.comare equivalent.Apply this diff:
- return allowListSetting - ? allowListSetting - .split(',') - .map((d) => d.trim().toLowerCase()) - .filter(Boolean) - : null; + return allowListSetting + ? allowListSetting + .split(',') + .map((d) => d.trim().toLowerCase().replace(/^\.+/, '').replace(/\.$/, '')) + .filter(Boolean) + : null;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (4)
apps/meteor/server/settings/federation-service.ts(1 hunks)ee/packages/federation-matrix/src/FederationMatrix.ts(2 hunks)ee/packages/federation-matrix/src/api/middlewares/isFederationDomainAllowed.ts(1 hunks)packages/i18n/src/locales/en.i18n.json(2 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-19T15:15:04.613Z
Learnt from: rodrigok
PR: RocketChat/Rocket.Chat#36991
File: apps/meteor/server/services/federation/infrastructure/rocket-chat/adapters/Settings.ts:219-221
Timestamp: 2025-09-19T15:15:04.613Z
Learning: The Federation_Matrix_homeserver_domain setting in apps/meteor/server/services/federation/infrastructure/rocket-chat/adapters/Settings.ts is part of the old federation system and is being deprecated/removed, so configuration issues with this setting should not be flagged for improvement.
Applied to files:
ee/packages/federation-matrix/src/api/middlewares/isFederationDomainAllowed.tsee/packages/federation-matrix/src/FederationMatrix.tsapps/meteor/server/settings/federation-service.ts
🧬 Code graph analysis (1)
ee/packages/federation-matrix/src/FederationMatrix.ts (1)
ee/packages/federation-matrix/src/api/middlewares/isFederationDomainAllowed.ts (1)
isFederationDomainAllowedMiddleware(36-61)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: 📦 Build Packages
- GitHub Check: CodeQL-Build
- GitHub Check: CodeQL-Build
🔇 Additional comments (2)
ee/packages/federation-matrix/src/FederationMatrix.ts (1)
24-24: Import looks good.ee/packages/federation-matrix/src/api/middlewares/isFederationDomainAllowed.ts (1)
6-17: Caching approach is fine.60s TTL via
memis reasonable for settings churn.
| function parseMatrixAuthorizationHeader(header: string): Record<string, string> { | ||
| const result: Record<string, string> = {}; | ||
| // Match key="value" pairs | ||
| const regex = /([a-zA-Z0-9_-]+)\s*=\s*"([^"]*)"/g; | ||
| let match; | ||
| while ((match = regex.exec(header)) !== null) { | ||
| result[match[1]] = match[2]; | ||
| } | ||
| return result; | ||
| } |
There was a problem hiding this comment.
Header parsing stores case-sensitive keys; normalize to lowercase to avoid origin misses.
If a peer sends Origin= (capitalized), authValues.origin will be undefined.
Apply this diff:
- while ((match = regex.exec(header)) !== null) {
- result[match[1]] = match[2];
- }
+ while ((match = regex.exec(header)) !== null) {
+ result[match[1].toLowerCase()] = match[2];
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| function parseMatrixAuthorizationHeader(header: string): Record<string, string> { | |
| const result: Record<string, string> = {}; | |
| // Match key="value" pairs | |
| const regex = /([a-zA-Z0-9_-]+)\s*=\s*"([^"]*)"/g; | |
| let match; | |
| while ((match = regex.exec(header)) !== null) { | |
| result[match[1]] = match[2]; | |
| } | |
| return result; | |
| } | |
| function parseMatrixAuthorizationHeader(header: string): Record<string, string> { | |
| const result: Record<string, string> = {}; | |
| // Match key="value" pairs | |
| const regex = /([a-zA-Z0-9_-]+)\s*=\s*"([^"]*)"/g; | |
| let match; | |
| while ((match = regex.exec(header)) !== null) { | |
| result[match[1].toLowerCase()] = match[2]; | |
| } | |
| return result; | |
| } |
🤖 Prompt for AI Agents
In
ee/packages/federation-matrix/src/api/middlewares/isFederationDomainAllowed.ts
around lines 25 to 34, the parseMatrixAuthorizationHeader function stores header
keys with original casing so headers like "Origin" become inaccessible as
"origin"; update the parser to normalize header keys to lowercase when storing
(e.g. use match[1].toLowerCase()) and ensure trimming any surrounding whitespace
before lowercasing so subsequent lookups like authValues.origin succeed
reliably.
| const authValues = parseMatrixAuthorizationHeader(authHeader); | ||
| const domain = authValues.origin?.toLowerCase(); | ||
| if (!domain) { | ||
| return c.json({ errcode: 'M_MISSING_ORIGIN', error: 'Missing origin in authorization header.' }, 401); | ||
| } |
There was a problem hiding this comment.
Normalize server name: strip port/brackets; handle IPv6; lowercase.
origin may include a port (e.g., example.org:8448) or IPv6 ([2001:db8::1]:8448); current code will fail matching.
Apply this diff:
+function normalizeServerName(input: string): string | undefined {
+ if (!input) return undefined;
+ let host = input.trim().toLowerCase();
+ // IPv6 in brackets: [::1]:8448
+ if (host.startsWith('[')) {
+ const end = host.indexOf(']');
+ if (end > 0) host = host.slice(1, end);
+ } else {
+ // Strip :port for IPv4/FQDN
+ const idx = host.indexOf(':');
+ if (idx > -1) host = host.slice(0, idx);
+ }
+ // Remove trailing dot
+ return host.replace(/\.$/, '');
+}
@@
- const authValues = parseMatrixAuthorizationHeader(authHeader);
- const domain = authValues.origin?.toLowerCase();
+ const authValues = parseMatrixAuthorizationHeader(authHeader);
+ const domain = normalizeServerName(authValues.origin);
if (!domain) {
return c.json({ errcode: 'M_MISSING_ORIGIN', error: 'Missing origin in authorization header.' }, 401);
}Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In
ee/packages/federation-matrix/src/api/middlewares/isFederationDomainAllowed.ts
around lines 49-53, the origin from parseMatrixAuthorizationHeader may include a
port or IPv6 brackets and must be normalized before use: strip surrounding
brackets for IPv6, remove an appended port (e.g., "host:8448" -> "host",
"[2001:db8::1]:8448" -> "2001:db8::1"), and lowercase the result; implement this
by checking if origin starts with '[' then extract the substring between '[' and
']', else if origin contains any ':' decide whether it has a port by testing for
a single colon (split on last ':' and take the left-hand side) vs multiple
colons (treat as IPv6 without port and keep whole string), then lowercase and
continue with the existing null/empty check and response.
| // Check if domain is in allowed list | ||
| if (allowList.some((allowed) => domain.endsWith(allowed))) { | ||
| return next(); | ||
| } |
There was a problem hiding this comment.
Fix suffix matching to respect label boundaries (security).
endsWith(allowed) lets notdevilevil.com match evil.com. Require exact match or . boundary.
Apply this diff:
- if (allowList.some((allowed) => domain.endsWith(allowed))) {
+ if (allowList.some((allowed) => domain === allowed || domain.endsWith(`.${allowed}`))) {
return next();
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Check if domain is in allowed list | |
| if (allowList.some((allowed) => domain.endsWith(allowed))) { | |
| return next(); | |
| } | |
| // Check if domain is in allowed list | |
| if (allowList.some((allowed) => domain === allowed || domain.endsWith(`.${allowed}`))) { | |
| return next(); | |
| } |
🤖 Prompt for AI Agents
In
ee/packages/federation-matrix/src/api/middlewares/isFederationDomainAllowed.ts
around lines 55-58, the current check uses domain.endsWith(allowed) which allows
suffix matches across label boundaries (e.g., notdevilevil.com matching
evil.com); change the check to require either an exact match or a dot boundary
by replacing the condition with a normalized comparison like: lowercase/trim
both domain and allowed, then return next() only if domain === allowed ||
domain.endsWith('.' + allowed). This enforces host-label boundaries and avoids
false positive suffix matches.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## chore/federation-backup #37010 +/- ##
==========================================================
Coverage ? 69.83%
==========================================================
Files ? 3033
Lines ? 103299
Branches ? 18355
==========================================================
Hits ? 72142
Misses ? 29281
Partials ? 1876
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
Proposed changes (including videos or screenshots)
Issue(s)
FDR-128
Steps to test or reproduce
Further comments
Summary by CodeRabbit